Skip to content

chore: clear pending timers on destroy + clarify reflow/highlight intent#473

Merged
gnbm merged 9 commits into
perf/throttle-resize-handlerfrom
chore/timer-cleanup-hardening
Jun 8, 2026
Merged

chore: clear pending timers on destroy + clarify reflow/highlight intent#473
gnbm merged 9 commits into
perf/throttle-resize-handlerfrom
chore/timer-cleanup-hardening

Conversation

@gnbm

@gnbm gnbm commented Jun 3, 2026

Copy link
Copy Markdown
Collaborator

Problem 1 — clear pending timers on destroy

  • Several setTimeout(...) callbacks (deferred renderOptions, search reset, focus) were fire-and-forget and not cleared on destroy(), so they could run against a destroyed instance. They now go through a tracked setManagedTimeout(), and destroy() calls clearManagedTimeouts().

Problem 2 — document the intentional forced reflow

  • openDropbox reads offsetHeight to flush a transition: none. Added a comment so it isn't "optimised away" as a no-op.

Problem 3 — document the search-highlight escaping assumption

  • The <mark> highlight regex already regex-escapes the search input (no ReDoS) and relies on labels being escaped via enableSecureText. Added a comment clarifying it's not an extra injection vector.

Verification

New spec cypress/e2e/timer-cleanup.cy.ts:

  • a pending managed timeout does not fire after destroy();
  • destroying right after an option select doesn't throw and clears the instance reference.

Other Validations:

  • Ran npm run validate for static code validation: image
  • npm run build: succeeds; dist/docs rebuilt
  • All tests passed (ran npm run test ):
image

…ent (S4/P3/P4)

P4: route fire-and-forget setTimeout calls through setManagedTimeout, whose ids are
tracked and cleared in destroy(). Prevents callbacks (e.g. renderOptions, focus)
from running against a destroyed instance.

P3: document the intentional forced reflow in openDropbox so it is not removed as a
no-op.

S4: document that the search-highlight regex relies on enableSecureText for escaping
and is not an additional injection vector (input is already regex-escaped).

Adds cypress spec timer-cleanup.cy.ts.
gnbm added 2 commits June 4, 2026 00:12
Add JSDoc types to Utils.throttle (typed timeout and lastArgs, and typed throttled args) and switch callback.apply to a spread invocation to simplify typing. Also initialize lastArgs as an array instead of null. In the test, remove an extraneous @ts-expect-error comment. These tweaks improve TypeScript/JSDoc compatibility and tidy up the test file.
Add JSDoc types for timeout and lastArgs and initialize lastArgs as an array. Replace callback.apply(this, lastArgs) with callback(...lastArgs) in Utils.throttle to simplify invocation and improve type-awareness. Updated built artifacts and docs assets (dist and docs/assets) accordingly.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens VirtualSelect teardown by tracking and clearing deferred setTimeout work during destroy(), and adds clarifying comments around intentional forced reflow and the search highlighting escape assumptions. It also adds Cypress coverage for the timer cleanup behavior and updates built/dist assets accordingly.

Changes:

  • Route previously “fire-and-forget” instance timers through setManagedTimeout() and clear them during destroy().
  • Add/expand inline documentation for the intentional forced reflow on open and for the <mark> search highlighting escape assumptions.
  • Add Cypress e2e coverage validating timeouts don’t fire post-destroy and that destroy-after-select doesn’t throw.

Reviewed changes

Copilot reviewed 4 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/virtual-select.js Adds managed timeout tracking/cleanup; clarifies reflow/highlight intent; clears timers on destroy.
src/utils/utils.js Updates throttle() implementation and JSDoc typing around its timer/args handling.
cypress/e2e/timer-cleanup.cy.ts Adds e2e coverage ensuring managed timeouts are canceled on destroy and no regression on destroy-after-select.
docs/assets/virtual-select.js Rebuilt docs bundle reflecting the managed timeout and comment changes.
docs/assets/virtual-select.min.js Rebuilt minified docs bundle reflecting the managed timeout and comment changes.
dist/virtual-select.js Rebuilt distribution bundle reflecting the managed timeout and comment changes.
dist/virtual-select.min.js Rebuilt minified distribution bundle reflecting the managed timeout and comment changes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/utils/utils.js Outdated
Comment thread src/utils/utils.js Outdated
Comment thread src/virtual-select.js

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 8 changed files in this pull request and generated 2 comments.

Comment thread src/utils/utils.js Outdated
Comment thread src/utils/utils.js Outdated
Capture and preserve the calling context in Utils.throttle by converting the returned function to a named function, adding JSDoc for `this`/args, storing `this` to `context`, and invoking the callback with `callback.apply(context, lastArgs)`. Return the throttled function explicitly. Also reword a comment in virtual-select to clarify that turning off enableSecureText is an explicit consumer choice to render raw HTML, so the added highlight does not introduce an extra injection vector.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 8 changed files in this pull request and generated 1 comment.

Comment thread docs/assets/virtual-select.js

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 8 changed files in this pull request and generated 1 comment.

Comment thread docs/assets/virtual-select.js

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 8 changed files in this pull request and generated 4 comments.

Comment thread src/utils/utils.js Outdated
Comment thread src/utils/utils.js Outdated
Comment thread docs/assets/virtual-select.js
Comment thread cypress/e2e/timer-cleanup.cy.ts
gnbm added 2 commits June 8, 2026 14:24
- Remove unused 'const context = this' and unreachable out-of-scope
  'return throttled;' in Utils.throttle (Copilot review). Source had
  drifted from the already-correct dist bundle; restores parity, no
  behavior change.
- Use cy.clock()/cy.tick() in timer-cleanup test so the negative
  'timer must not fire after destroy' assertion is deterministic and
  not masked by CI timer throttling (Copilot review).
Regenerated dist/docs bundles were out of sync with an already-committed
source comment rewording. No code/behavior change; throttle output is
unchanged. Separated from the fix commit since it is pre-existing drift.
@gnbm gnbm requested a review from Copilot June 8, 2026 13:26
@gnbm gnbm marked this pull request as ready for review June 8, 2026 13:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 7 changed files in this pull request and generated 1 comment.

Comment thread docs/assets/virtual-select.js

@os-davidlourenco os-davidlourenco left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gnbm gnbm merged commit 54028b4 into perf/throttle-resize-handler Jun 8, 2026
1 check passed
gnbm added a commit that referenced this pull request Jun 8, 2026
…ent (#473)

* chore: clear pending timers on destroy + clarify reflow/highlight intent (S4/P3/P4)

P4: route fire-and-forget setTimeout calls through setManagedTimeout, whose ids are
tracked and cleared in destroy(). Prevents callbacks (e.g. renderOptions, focus)
from running against a destroyed instance.

P3: document the intentional forced reflow in openDropbox so it is not removed as a
no-op.

S4: document that the search-highlight regex relies on enableSecureText for escaping
and is not an additional injection vector (input is already regex-escaped).

Adds cypress spec timer-cleanup.cy.ts.

* Improve throttle typings and clean test comment

Add JSDoc types to Utils.throttle (typed timeout and lastArgs, and typed throttled args) and switch callback.apply to a spread invocation to simplify typing. Also initialize lastArgs as an array instead of null. In the test, remove an extraneous @ts-expect-error comment. These tweaks improve TypeScript/JSDoc compatibility and tidy up the test file.

* Refactor Utils.throttle to use spread args

Add JSDoc types for timeout and lastArgs and initialize lastArgs as an array. Replace callback.apply(this, lastArgs) with callback(...lastArgs) in Utils.throttle to simplify invocation and improve type-awareness. Updated built artifacts and docs assets (dist and docs/assets) accordingly.

* Update virtual-select-1.2.0.min.js

* Preserve context in throttle and clarify comment

Capture and preserve the calling context in Utils.throttle by converting the returned function to a named function, adding JSDoc for `this`/args, storing `this` to `context`, and invoking the callback with `callback.apply(context, lastArgs)`. Return the throttled function explicitly. Also reword a comment in virtual-select to clarify that turning off enableSecureText is an explicit consumer choice to render raw HTML, so the added highlight does not introduce an extra injection vector.

* fix(utils): remove broken throttle edit; harden timer-cleanup test

- Remove unused 'const context = this' and unreachable out-of-scope
  'return throttled;' in Utils.throttle (Copilot review). Source had
  drifted from the already-correct dist bundle; restores parity, no
  behavior change.
- Use cy.clock()/cy.tick() in timer-cleanup test so the negative
  'timer must not fire after destroy' assertion is deterministic and
  not masked by CI timer throttling (Copilot review).

* build: sync bundles with committed source (stale highlight comment)

Regenerated dist/docs bundles were out of sync with an already-committed
source comment rewording. No code/behavior change; throttle output is
unchanged. Separated from the fix commit since it is pre-existing drift.
@gnbm gnbm mentioned this pull request Jun 8, 2026
gnbm added a commit that referenced this pull request Jun 8, 2026
* perf: throttle global resize handler and guard against instance-less wrappers (P1)

The window 'resize' listener ran onResizeMethod on every tick, which queried all
.vscomp-ele-wrapper elements and recomputed each instance's options-container
height - O(instances) layout work per resize event during a drag. It also assumed
every wrapper's parent had a .virtualSelect and threw otherwise.

- Add Utils.throttle and wrap the resize listener (~100ms) so the per-instance
  work runs at most ~10x/sec.
- Guard onResizeMethod against wrappers whose instance is missing (teardown races).

Closed instances are still updated (opening does not recompute height, so skipping
them could leave a stale height on reopen) - throttling delivers the win without
that regression risk.

Adds cypress spec perf-resize-throttle.cy.ts (throttling, no-regression, guard).

* Update utils.js

Fix lint issues

* Add typings to throttle and use spread

Add JSDoc/type annotations to Utils.throttle (timeout and lastArgs) and annotate the throttled function parameter. Replace callback.apply(this, lastArgs) with callback(...lastArgs) for simpler invocation and improved type-safety. Updated the built/minified distribution and docs asset files accordingly.

* fix(utils): preserve caller context in throttle (Copilot review)

Invoke the throttled callback via callback.apply(lastThis, lastArgs) on
both leading and trailing edges so instance methods used as throttled
event handlers run with the expected 'this'. Add @this annotation to
satisfy tsc. Regenerate dist/docs bundles.

* fix: prevent Virtual Select memory leaks (document click listener + cleanup gaps) (#468)

* fix: remove document click listener correctly to prevent memory leak

onDocumentClick is added to `document` in the capture phase
(addEvent(document, 'click', 'onDocumentClick', true)), but removeEvent called
removeEventListener without the capture flag. removeEventListener only
unregisters a listener when its capture flag matches the one used at
registration, so the capture-phase document listener was never removed.

As a result, every render/destroy cycle (e.g. repeatedly opening and closing
the dropdown) left a new onDocumentClick listener bound to `document`. Each
leaked listener retains the VirtualSelect instance and its detached dropbox DOM
subtree, so event-listener and DOM-node counts grow without bound and are never
reclaimed by GC.

Fix: thread the capture flag through DomUtils.removeEvent and the
VirtualSelect.removeEvent wrapper, and pass capture: true when removing the
document click listener so add and remove match.

* fix: ensure cleanup runs in inline mode and clear retained references on destroy

Two additional memory-leak hardening fixes:

1. Install the self-destroy MutationObserver in every mode, not only when
   hasDropboxWrapper is true. Previously, inline-mode dropdowns had no observer,
   so removing the host element from the DOM without calling destroy() left all
   addEvents() listeners attached - including the capture-phase document click
   listener that retains the instance and its detached DOM. removeMutationObserver
   now disconnects based on the observer's existence.

2. On destroy(), clear the $dropboxWrapper.virtualSelect back-reference (set in
   setEleProps) before detaching the wrapper, and reset this.events. This breaks
   the instance <-> detached-DOM reference cycle so the instance and its DOM can
   be reclaimed.

* fix: limit mutation observer to popup mode to avoid per-instance perf penalty

Reverts the all-modes MutationObserver install. In the default
dropboxWrapper:'self' mode every instance was registering a body-wide
subtree observer, adding O(instances x mutations) overhead on pages with
many selects. The reported document-click listener leak is fixed by the
capture-flag correction in removeEvents(); the observer stays scoped to
popup mode as before.

* build: rebuild minified bundles with S2/S3/P1 fixes

The branch committed the unminified dist/docs sources and the dist-archive
copy with the security (S2 customData escaping, S3 classNames sanitization)
and perf (P1 throttled resize) fixes, but the primary distributed bundles
(dist/virtual-select.min.js and docs/assets/virtual-select.min.js) were left
stale - consumers loading them got none of the fixes.

Regenerated via 'npm run build' so all three min bundles share an identical
body and match the source.

* refactor: keep stable reference to throttled resize handler (Copilot review)

The throttled wrapper passed inline to addEventListener was anonymous, so the
resize listener could no longer be detached via removeEventListener (the prior
VirtualSelect.onResizeMethod reference was removable). Store the throttled
function on VirtualSelect.onResizeThrottled and register that reference instead,
restoring removability for future cleanup. Declared as a typed static field so
it passes tsc (checkJs + strict).

* Potential fix for pull request finding

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

* Revert "Potential fix for pull request finding"

This reverts commit 98c9649.

* refactor: drop unnecessary static class field for onResizeThrottled (Copilot review)

The static class-field declaration provided no type-safety benefit (the file
is // @ts-nocheck, so tsc never checks it) and used ES2022 class-field syntax
not present anywhere else in the codebase. The property is assigned at module
init time via VirtualSelect.onResizeThrottled = ..., which is sufficient to
expose a stable, removable reference. Runtime behaviour is unchanged.

* fix(utils): release throttle references after invocation (Copilot review)

throttle retained lastArgs/lastThis (e.g. a DOM Event passed to a resize
handler) until the next call or indefinitely if never called again. Clear them
after both the leading and trailing invocations so large arguments aren't held,
matching the canonical underscore/lodash throttle. Behaviour is unchanged:
both paths reassign lastArgs/lastThis at the top of every call before use.

* chore: clear pending timers on destroy + clarify reflow/highlight intent (#473)

* chore: clear pending timers on destroy + clarify reflow/highlight intent (S4/P3/P4)

P4: route fire-and-forget setTimeout calls through setManagedTimeout, whose ids are
tracked and cleared in destroy(). Prevents callbacks (e.g. renderOptions, focus)
from running against a destroyed instance.

P3: document the intentional forced reflow in openDropbox so it is not removed as a
no-op.

S4: document that the search-highlight regex relies on enableSecureText for escaping
and is not an additional injection vector (input is already regex-escaped).

Adds cypress spec timer-cleanup.cy.ts.

* Improve throttle typings and clean test comment

Add JSDoc types to Utils.throttle (typed timeout and lastArgs, and typed throttled args) and switch callback.apply to a spread invocation to simplify typing. Also initialize lastArgs as an array instead of null. In the test, remove an extraneous @ts-expect-error comment. These tweaks improve TypeScript/JSDoc compatibility and tidy up the test file.

* Refactor Utils.throttle to use spread args

Add JSDoc types for timeout and lastArgs and initialize lastArgs as an array. Replace callback.apply(this, lastArgs) with callback(...lastArgs) in Utils.throttle to simplify invocation and improve type-awareness. Updated built artifacts and docs assets (dist and docs/assets) accordingly.

* Update virtual-select-1.2.0.min.js

* Preserve context in throttle and clarify comment

Capture and preserve the calling context in Utils.throttle by converting the returned function to a named function, adding JSDoc for `this`/args, storing `this` to `context`, and invoking the callback with `callback.apply(context, lastArgs)`. Return the throttled function explicitly. Also reword a comment in virtual-select to clarify that turning off enableSecureText is an explicit consumer choice to render raw HTML, so the added highlight does not introduce an extra injection vector.

* fix(utils): remove broken throttle edit; harden timer-cleanup test

- Remove unused 'const context = this' and unreachable out-of-scope
  'return throttled;' in Utils.throttle (Copilot review). Source had
  drifted from the already-correct dist bundle; restores parity, no
  behavior change.
- Use cy.clock()/cy.tick() in timer-cleanup test so the negative
  'timer must not fire after destroy' assertion is deterministic and
  not masked by CI timer throttling (Copilot review).

* build: sync bundles with committed source (stale highlight comment)

Regenerated dist/docs bundles were out of sync with an already-committed
source comment rewording. No code/behavior change; throttle output is
unchanged. Separated from the fix commit since it is pre-existing drift.

---------

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants